Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/vm: implement EIP-2681: Limit account nonce to 2^64-1 #23853

Merged
merged 4 commits into from
Nov 11, 2021

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Nov 3, 2021

This retroactively implements requirements or EIP-2681 for the account nonce upper limit.
Consensus tests (covering only CREATE/CREATE2 case): ethereum/tests#934

Opening PR as requested in ethereum/tests#934 (comment)

Required by EIP-2681: Limit account nonce to 2^64-1
@holiman
Copy link
Contributor

holiman commented Nov 3, 2021

What about the case where the EOA goes above the same limit, shouldn't the tx be rejected in that case? Do we already have that in place?

@gumb0
Copy link
Member Author

gumb0 commented Nov 4, 2021

What about the case where the EOA goes above the same limit, shouldn't the tx be rejected in that case? Do we already have that in place?

Yes it should, tx_pool doesn't have such check, but nonces are already uint64 there. Digging deeper to find where it's decoded...

@gumb0
Copy link
Member Author

gumb0 commented Nov 4, 2021

Here I found another place where the check is missing when external transaction updates the sender's nonce.

st.state.SetNonce(msg.From(), st.state.GetNonce(sender.Address())+1)

EIP is not very clear for this case, (it only mentions tx nonce being above 64 bit, which is already rejected by geth as invalid tx encoding) I think it would make sense for a transaction to be valid and fail.

cc @axic

@gumb0
Copy link
Member Author

gumb0 commented Nov 4, 2021

And creation transaction BTW goes through the same code path as CREATE/CREATE2 and with this PR should already result in failed execution

ret, _, st.gas, vmerr = st.evm.Create(sender, st.data, st.gas, st.value)

@holiman
Copy link
Contributor

holiman commented Nov 4, 2021 via email

@axic
Copy link
Member

axic commented Nov 4, 2021

EIP is not very clear for this case, (it only mentions tx nonce being above 64 bit, which is already rejected by geth as invalid tx encoding) I think it would make sense for a transaction to be valid and fail.

The mempool is not part of the EIP as clients are free to decide how they select transactions.

@gumb0
Copy link
Member Author

gumb0 commented Nov 4, 2021

EIP is not very clear for this case, (it only mentions tx nonce being above 64 bit, which is already rejected by geth as invalid tx encoding) I think it would make sense for a transaction to be valid and fail.

The mempool is not part of the EIP as clients are free to decide how they select transactions.

"is not very clear" did not refer to the mempool, but to external transaction updating sender's nonce when included.

@axic
Copy link
Member

axic commented Nov 4, 2021

EIP is not very clear for this case, (it only mentions tx nonce being above 64 bit, which is already rejected by geth as invalid tx encoding) I think it would make sense for a transaction to be valid and fail.

The mempool is not part of the EIP as clients are free to decide how they select transactions.

"is not very clear" did not refer to the mempool, but to external transaction updating sender's nonce when included.

Such transactions should not be accepted, i.e. they are invalid.

@gumb0
Copy link
Member Author

gumb0 commented Nov 4, 2021

"is not very clear" did not refer to the mempool, but to external transaction updating sender's nonce when included.

Such transactions should not be accepted, i.e. they are invalid.

This would make transactions with nonce == 0xffffffffffffffff invalid (and not only where the nonce exceeds 2^64-1 as EIP states)

@axic
Copy link
Member

axic commented Nov 4, 2021

This would make transactions with nonce == 0xffffffffffffffff invalid (and not only where the nonce exceeds 2^64-1 as EIP states)

I mean those which exceed are invalid.

@holiman
Copy link
Contributor

holiman commented Nov 4, 2021

This would make transactions with nonce == 0xffffffffffffffff invalid (and not only where the nonce exceeds 2^64-1 as EIP states)

I'd agree with that: by themselves they are valid, but since nonce increase is a consequence of including them, they lead to breaking the rule. Hence they must be rejected and cannot be included in a block. (not in the txpool though, this is a consensus rule)

@axic
Copy link
Member

axic commented Nov 4, 2021

Hmm, that's an interesting point, that the increase is part of the inclusion, so transaction nonce 2^64-1 can't be included. We should perhaps clarify this in the EIP.

@holiman
Copy link
Contributor

holiman commented Nov 9, 2021

Triage discussion: we should fix both nonce-out-of-bounds cases in the same PR

@gumb0
Copy link
Member Author

gumb0 commented Nov 10, 2021

I think a change to evm transaction tool is also needed.

@holiman
Copy link
Contributor

holiman commented Nov 11, 2021

Hmpf...

ERROR: Permission to ewasm/go-ethereum.git denied to holiman.

Please cherry-pick from https://github.com/holiman/go-ethereum/commits/deletelater

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, if you also add the testcase from my branch

@holiman holiman added this to the 1.10.13 milestone Nov 11, 2021
@holiman holiman merged commit f32feeb into ethereum:master Nov 11, 2021
@axic axic deleted the eip-2681 branch November 11, 2021 14:01
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Nov 11, 2021
…23853)

This retroactively implements requirements or EIP-2681 for the account nonce upper limit.
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
…23853)

This retroactively implements requirements or EIP-2681 for the account nonce upper limit.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Aug 20, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Aug 25, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Aug 25, 2024
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants